Skip to content

[???] Add more automated build and test script#251

Open
MartinFlores751 wants to merge 15 commits intoirods:mainfrom
MartinFlores751:T-bats
Open

[???] Add more automated build and test script#251
MartinFlores751 wants to merge 15 commits intoirods:mainfrom
MartinFlores751:T-bats

Conversation

@MartinFlores751
Copy link
Contributor

This PR introduces a script that allows for the automation of building and testing through either a JSON input, or an interactive prompt.

An example of the current structure for building and testing is as follows:

{
    "build": [
            {
                    "args": ["--ccache", "--enable-address-sanitizer"],
                    "os": "ubuntu-20.04",
            },
            {
                    "args": ["--ninja"],
                    "os": "debian-12",
            }
    ],
    "test": [
            {
                    "args": ["--concurrent-test-executor-count", "4", "--discard-logs"],
                    "os": "ubuntu-20.04",
                    "db": "mariadb-10.6",
                    "type": "run_core_tests"
            },
            {
                    "args": ["--concurrent-test-executor-count", "2", "--discard-logs", "--tests", "some.Test.Here"],
                    "os": "ubuntu-20.04",
                    "db": "mariadb-10.6",
                    "type": "run_unit_tests"
            },
            {
                    "args": ["--discard-logs"],
                    "os": "debian-12",
                    "db": "mariadb-10.11",
                    "type": "run_federation_tests"
            }
    ]
}

Some things in this script can be trimmed to be more focused on only building and testing (e.g., cutting the refresh_*_buliders(...)).

This script does assume a certain directory structure. This can be modified, possibly on a per-user basis, but a 'standard' structure might help.

Further comments will be left in code review shortly...

@MartinFlores751
Copy link
Contributor Author

The script also requires you to run in the same virtualenv used for the testing environment scripts.

Copy link
Contributor Author

@MartinFlores751 MartinFlores751 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointed out some issues with the script off the top of my head.

# Source directories
IRODS_SERV_DIR = IRODS_BASE_DIR / 'irods'
IRODS_ICOMMANDS_DIR = IRODS_BASE_DIR / 'irods_client_icommands'
IRODS_PLUGINS_SRC_DIR = IRODS_BASE_DIR # / 'plugins'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment.

Comment on lines +186 to +192
named_args.append(('--plugin-package-directory', str(IRODS_PLUGIN_PACKAGE_DIR).format(os_name=os_choice, short_plugin_name=plugin_name)))
elif desired_test.stem == 'run_core_tests':
num_executors = int(input('How many test executors do you want? '))
named_args.append(('--concurrent-test-executor-count', str(num_executors)))
elif desired_test.stem == 'run_topology_tests':
num_executors = int(input('How many test executors do you want? '))
named_args.append(('--concurrent-test-executor-count', str(num_executors)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting here, but it applies elsewhere, we should separate the interactive part so the validation/work can be entirely separate from the prompts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be made into a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document any exceptions that still need to be handled for future issues.

  • Failure to find SHA

@trel
Copy link
Member

trel commented Mar 4, 2025

In general... i think a standard/assumed/prescriptive directory structure will cut down on much confusion.

@@ -0,0 +1,832 @@
#! /usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add words about expected inputs and outputs for this script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay to put this in a --help, or should it (also) be here at the top?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--help is fine. easy to copy/paste to the most helpful place later, as long as it exists.

@alanking
Copy link
Contributor

alanking commented Mar 4, 2025

In general... i think a standard/assumed/prescriptive directory structure will cut down on much confusion.

Or... we make a way to configure it, at least in part. I keep all my packages in a mount point to my spinning disk to prevent filling up my SSD, so being able to move around the base directory would be very helpful. I guess I could also just, y'know, modify the file. But that's annoying. :)

@trel
Copy link
Member

trel commented Mar 4, 2025

being able to move around the base directory would be very helpful

yes, the location itself should be configurable...

but the directory structure within... i think we should strongly consider 'fixing' to the one-true-way.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad someone finally got angry enough to make something like this. It was always the objective, just took a while to get here. :)

Definitely going to need to see a demo or some README documentation so I can get a better handle on the big picture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants